Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(citext): support postgres citext #2478

Merged
merged 7 commits into from
Oct 12, 2023

Conversation

hgranthorner
Copy link
Contributor

Resolves #295

Users needed to explicitly cast all CITEXT columns to TEXT. This set of changes will cause CITEXT columns to be treated the same as TEXT columns by sqlx.

Happy to add additional details/code/comments/tests as needed.

@abonander
Copy link
Collaborator

Looks like you need to add CREATE EXTENSION IF NOT EXISTS citext; here: https://github.com/launchbadge/sqlx/blob/main/tests/postgres/setup.sql#L3

@hgranthorner
Copy link
Contributor Author

@abonander thanks for pointing that out. I've went ahead and updated the postgres setup.sql file.

@abonander
Copy link
Collaborator

@hgranthorner CI is still failing.

@hgranthorner hgranthorner force-pushed the add-citext-support-postgres branch from e881490 to ededdcc Compare May 14, 2023 18:14
@hgranthorner hgranthorner force-pushed the add-citext-support-postgres branch from ededdcc to 133c251 Compare May 14, 2023 18:15
@hgranthorner
Copy link
Contributor Author

So my previous implementation of this was completely wrong. I assumed that because it worked for me, it would work for everyone, and due to some test setup issues (more on that below) I wasn't able to figure out how to test it beyond just running some samples. The new implementation follows the ltree implementation by dynamically looking up its own oid at run time when needed. Note that while Postgres is happy to compare citext and text, it refuses to compare citext[] and text[]. It seems like you have already bumped into that, since you there are no test cases for varchar[] -> Vec<String>, etc. It seems like this would break some of sqlx's compile time checking though. I also double checked that everything would work without the citext extension installed (seems like everything worked), but didn't know how to set up test cases for that. I wasn't sure if I should hide this functionality behind a new citext flag - happy to add that if you'd like.

In terms of the testing issues, I ran into 2. I'm fairly certain that both of them are due to me being on Windows. For the first, the tests/docker.py file attempts to inspect the running docker instances for f"sqlx_{driver}_1". On my machine, the images were named sqlx-{driver}-1. This failure was giving me a pretty inscrutable error. If someone else can confirm that this is intended docker compose functionality on Windows, I'm happy to raise a PR to fix this for others - I was unable to find any documentation on it anywhere. My other issue had to do with some weird configuration I had set up around Docker with a WSL backend - Docker tends to devour RAM with a WSL backend for some reason, so I had set memory limits for the application. During sqlx testing, the SUTs would start closing client connections with no mention of OOM or anything like that. Removing the limitations fixed that for me. I still wasn't able to run the any tests, but I think I'm just messing something up locally...

Long story short - sorry for wasting your time earlier. Things should (hopefully) be good now. Glad to take any and all feedback.

@abonander
Copy link
Collaborator

Sorry for the delay. I have a few nits, but I'm just going to commit them as suggestions to avoid any more delays in getting this merged.

* Rename `PgCitext` to `PgCiText`
* Document when use of `PgCiText` is warranted
* Document potentially surprising `PartialEq` behavior
* Test that the macros consider `CITEXT` to be compatible with `String` and friends
@abonander abonander merged commit 540baf7 into launchbadge:main Oct 12, 2023
64 checks passed
anshap1719 pushed a commit to anshap1719/sqlx that referenced this pull request Oct 12, 2023
* feat(citext): implement citext for postgres

* feat(citext): add citext -> String conversion test

* feat(citext): fix ltree -> citree

* feat(citext): add citext to the setup.sql

* chore: address nits to launchbadge#2478

* Rename `PgCitext` to `PgCiText`
* Document when use of `PgCiText` is warranted
* Document potentially surprising `PartialEq` behavior
* Test that the macros consider `CITEXT` to be compatible with `String` and friends

* doc: add `PgCiText` to `postgres::types` listing

* chore: restore missing trailing line break to `tests/postgres/setup.sql`

---------

Co-authored-by: Austin Bonander <austin@launchbadge.com>
@anukul
Copy link

anukul commented Oct 23, 2023

@abonander Is there a timeline to release this on crates.io?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[PostgreSQL] citext is not compatable with text
3 participants